new checker http-const
#155
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This probably deserves a bit of refactoring. |
Pull Request Test Coverage Report for Build 9659794485Details
💛 - Coveralls |
ccoVeille
approved these changes
Jun 25, 2024
| @@ -56,9 +56,14 @@ func isTypedIntNumber(e ast.Expr, v int, types ...string) bool { | |||
| return false | |||
| } | |||
|
|
|||
| func isIntNumber(e ast.Expr, v int) bool { | |||
| func safeTypedBasicLit(e ast.Expr, typ token.Token) (*ast.BasicLit, bool) { | |||
There was a problem hiding this comment.
I'm unsure about the name, but thanks I had also noticed this could be refactored into a dedicated function
There was a problem hiding this comment.
As it is a type-safe BasicLit, I named it this way .
Feel free to change it if you have a better idea
There was a problem hiding this comment.
I'm so lame at wording. I'm only able to argue when I find a wording, but I'm never able to find a descent one to suggest 😅
| } | ||
|
|
||
| var httpStatusCode = map[string]string{ | ||
| strconv.Itoa(http.StatusContinue): "http.StatusContinue", |
There was a problem hiding this comment.
I don't think we should consider performance for this
I would have used a httpStatusCode = map[int]string{
so no, strconv.Itoa here
but only when needed in the code, you can also create a dedicated testing helper if your prefer
but current code looks strange to me
There was a problem hiding this comment.
That's just the way it's done in usestdlibvars, see
There was a problem hiding this comment.
I assumed it 😅. The code is so strange 😁
There was a problem hiding this comment.
Maybe Anton will provide his guidance
There was a problem hiding this comment.
@Antonboom it's me the serial mention guy 😅😁
What are your thoughts about this?
The fact the code imported from usedstdvars is a bit strange
mmorel-35
commented
Jun 25, 2024
internal/checkers/http_const.go
Outdated
| } | ||
| } | ||
| if len(suggestedFixes) > 0 { | ||
| return &analysis.Diagnostic{ |
There was a problem hiding this comment.
It might be better if newDiagnostic func was able to handle an array of suggestedFix instead of one . But that's for a dedicated PR.
Pull Request Test Coverage Report for Build 9668183301Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9668248204Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9668312351Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9668623785Details
💛 - Coveralls |
ccoVeille
approved these changes
Jun 25, 2024
internal/checkers/http_const.go
Outdated
| } | ||
| if bt, ok := safeTypedBasicLit(call.Args[4], token.INT); ok { | ||
| currentVal := bt.Value | ||
| key := strings.ToUpper(currentVal) |
There was a problem hiding this comment.
I was looking for this, please add tests for for lower case "get" or Pascal case (Get) and all other methods
There was a problem hiding this comment.
Between testify assert Method, http method and http status, that’s going to be a huge matrix to cover, any idea on how to cover it with a simple solution ?
Pull Request Test Coverage Report for Build 9669768290Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9669932292Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9669999628Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9670110548Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9673263571Details
💛 - Coveralls |
ccoVeille
reviewed
Jun 26, 2024
|
I now understand why @ldez (hi 👋) expects PR to do not rewriting history during code review. It's something I have been doing for a while, even in this project. But now I'm facing the way @mmorel-35 does, which I would have done too, I can see how difficult it is yo track the changes made between each iterations. Here there is only one commit, and I have to read everything again and again instead of seeing the small changes made by each reviews I repeat it's something I have done many times, even recently, so I don't blame you @mmorel-35, but now I understand Ludovic's guidance |
ccoVeille
suggested changes
Jun 26, 2024
|
@mmorel-35 I have no idea what you changed with your last commit. I reviewed everything and spotted things I found. But I have no idea if it's new code or not. |
Pull Request Test Coverage Report for Build 9674262358Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9674319119Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9674373889Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9674389614Details
💛 - Coveralls |
|
@ccoVeille , |
|
Thanks for taking my workflow suggestion into account. I do hate to see a log like that. But for reviewing it's way simpler. Please note, I'm OK with you to rearrange the commits in the way you intend. But many only once you get an approval for the PR. Anx with a message like in the PR like "squashing previously reviewd changes" I dislike squashed PR too, that's why I'm suggesting that. Also squashing when merging would nuke the respective contribution of each developer. Think about Anton "maintainer review" commit. At least, GitHub doesn't nuke authors who contributed as it adds the But I do think it's way easier for reviewing with atomic and iterative commits. Thanks again for baring with me |
ccoVeille
reviewed
Jun 27, 2024
Pull Request Test Coverage Report for Build 9695506618Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9696894260Details
💛 - Coveralls |
ccoVeille
suggested changes
Jun 27, 2024
internal/testgen/gen_http_const.go
Outdated
| {Fn: "HTTPStatusCode", Argsf: `httpOK, http.MethodGet, "/index", nil, http.StatusOK`}, | ||
| {Fn: "HTTPBodyContains", Argsf: `httpHelloName, http.MethodGet, "/", url.Values{"name": []string{"World"}}, "Hello, World!"`}, | ||
| }, | ||
| IgnoredAssertions: []Assertion{}, |
There was a problem hiding this comment.
@mmorel-35 I don't know if you saw this one
Pull Request Test Coverage Report for Build 9702352952Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9723300042Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9723821079Details
💛 - Coveralls |
ccoVeille
approved these changes
Jun 29, 2024
Pull Request Test Coverage Report for Build 9726718535Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9798875390Details
💛 - Coveralls |
ccoVeille
approved these changes
Jul 4, 2024
Pull Request Test Coverage Report for Build 9799153037Details
💛 - Coveralls |
ccoVeille
approved these changes
Jul 4, 2024
Pull Request Test Coverage Report for Build 9826249234Details
💛 - Coveralls |
2 similar comments
Pull Request Test Coverage Report for Build 9826249234Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9826249234Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9826674310Details
💛 - Coveralls |
2 similar comments
Pull Request Test Coverage Report for Build 9826674310Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9826674310Details
💛 - Coveralls |
ccoVeille
approved these changes
Jul 7, 2024
Pull Request Test Coverage Report for Build 10704164578Details
💛 - Coveralls |
Co-Authored-By: ccoVeille <[email protected]>
mmorel-35
force-pushed
the
http-const
branch
from
5004c06 to
1994ec7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #141